-
Notifications
You must be signed in to change notification settings - Fork 239
feat: mock_call with dynamic return data #2904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
0a08540
to
d75e50b
Compare
Add `mock_call_when`, `start_mock_call_when` and `stop_mock_call_when`
…:TargetCalls is 0
89e963b
to
02a7dcd
Compare
let key_zero = (call.entry_point_selector, Felt::zero()); | ||
|
||
match contract_functions.get(&key) { | ||
Some(CheatStatus::Cheated(_, CheatSpan::TargetCalls(0))) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After calling get_mocked_function_cheat_status
, we call .decrement_cheat_span
on the returned value. It changes the CheatSpan to Uncheated
after it decreases to 0
.
I don't think there can be a case where TargetCalls
is equal to 0 unless explicitly set to this value somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this logic match arm was added to handle the case where entrypoint for specific calldata is no longer cheated but for any calldata still is, but it should already be handled by _
case anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out technically it could be set to 0
but it shouldn't be allowed.
Created #2927
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand what should I do here: remove my workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to match for the case where cheat span is CheatSpan::TargetCalls(0)
. Technically, user could have somehow created it like this (as explained in #2927), but during normal operation it will either be CheatSpan::TargetCalls(some value > 0)
or CheatStatus::Uncheated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this
starknet-foundry/crates/cheatnet/src/state.rs
Line 141 in f2e7d0d
pub fn decrement_cheat_span(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workaround removed
crates/cheatnet/src/runtime_extensions/forge_runtime_extension/cheatcodes/mock_call.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also have test cases where mock_call_when
is mixed with mock_call
, these are the ones most likely to break/have some weird edge cases.
- rename cairo MockCallData enum into MockCalldata - use Serde derivation for MockCalldata instead of custom serializer.
} | ||
|
||
#[test] | ||
fn mock_calls_when_mixed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some cases where the same tests uses both mock_call
and mock_call_when
?
E.g. in case where you do start_mock_call_when(MockCalldata::Any, ...)
and then stop_mock_call(...)
and similar?
Hi! This pull request hasn't had any activity for a while, so I am |
Hey @ptisserand 👋 , are you going to continue working on this PR? |
Hi @franciszekjob, sorry I completely forgot about this PR when working on other projects :( |
@ptisserand sure thing, no hurry! |
Hi @ptisserand 👋 , kindly asking if there's any update on this? |
Hey @ptisserand is this ready for re-review? |
Not yet, I have only managed conflicts with master branch. |
Hi! This pull request hasn't had any activity for a while, so I am |
Hi @ptisserand 👋 , any update on this? |
Hi @franciszekjob, I have added some tests cases. |
Hi @cptartur @franciszekjob, could you please review this PR ? |
### `stop_mock_call_when` | ||
|
||
> `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### `stop_mock_call_when` | |
> `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` | |
## `stop_mock_call_when` | |
> `fn stop_mock_call_when(contract_address: ContractAddress, function_selector: felt252, calldata: MockCalldata)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CHANGELOG.md
Outdated
|
||
- Rust is no longer required to use `snforge` if using Scarb >= 2.10.0 on supported platforms - precompiled `snforge_scarb_plugin` plugin binaries are now published to [package registry](https://scarbs.xyz) for new versions. | ||
- Added a suggestion for using the `--max-n-steps` flag when the Cairo VM returns the error: `Could not reach the end of the program. RunResources has no remaining steps`. | ||
- `mock_call_when`, `start_mock_call_when`, `stop_mock_call_when` cheatcodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be under unreleased version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,43 @@ | |||
# `mock_call_when` | |||
|
|||
Cheatcodes mocking contract entry point calls: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the same as description of mock_call
. Maybe let's emphasize the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with Cheatcodes mocking contract entry point calls based on calldata:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really nice to add an example of usage, similarly how it's done here 😄
You can create and issue for that, link it here and if you have time open - a separate PR.
Closes #2861
Introduced changes
mock_call_when
,start_mock_call_when
andstop_mock_call_when
to support "dynamic" return value when mocking a contract entry point.Checklist
CHANGELOG.md